-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes SPARK-8730 - Deser objects containing a primitive class attribute #7122
Conversation
Test build #36127 has finished for PR 7122 at commit
|
Test build #36128 has finished for PR 7122 at commit
|
@@ -63,13 +63,30 @@ private[spark] class JavaDeserializationStream(in: InputStream, loader: ClassLoa | |||
|
|||
private val objIn = new ObjectInputStream(in) { | |||
override def resolveClass(desc: ObjectStreamClass): Class[_] = | |||
Class.forName(desc.getName, false, loader) | |||
try Class.forName(desc.getName, false, loader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap the try block w/ braces eg
try {
Class.forName(...)
} catch {
...
Test build #38351 has finished for PR 7122 at commit
|
} | ||
|
||
def readObject[T: ClassTag](): T = objIn.readObject().asInstanceOf[T] | ||
def close() { objIn.close() } | ||
} | ||
|
||
private object JavaSerDe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename this to match the class, JavaDeserializationStream
? Alternatively we could give it some name which is more specific, eg. JavaSerializationPrimitiveClassHelper
or something, but I think just making it a companion object is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I hesitated to do it at first but don't remember why I decided to not put it in a companion object. Will do
Test build #38394 has finished for PR 7122 at commit
|
@squito I don't get what fails. Is it some random issue or did I really break something? If I didn't should we just re run the tests? |
Test build #41828 has finished for PR 7122 at commit
|
@EugenCepoi yikes, sorry I forgot about this for so long! thanks for brining it up to date, merging to master |
Np, thanks for the review :) |
No description provided.